Add overflow detection to IntervalYearMonthOperators#24617
Add overflow detection to IntervalYearMonthOperators#24617pramodsatya merged 1 commit intoprestodb:masterfrom
Conversation
prestodb-ci
requested review from
a team,
ScrapCodes and
psnv03
and removed request for
a team
|
Thanks for the release note! New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR. I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link. |
|
Thanks @steveburnett, removed the manual PR link from release note. Could you please take another look? |
|
@hantangwangd, could you please help review/merge this PR? The commit is cherrypicked from #24089 that you had helped review before. Thanks! |
LGTM! Thanks! |
hantangwangd
left a comment
hantangwangd
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
|
@jaystarshot, @yingsu00, could you please help review this PR? |
|
@jaystarshot Can you please help with this review? Thanks! |
| return (long) (left * right); | ||
| long result = (long) (left * right); | ||
| if (result < Integer.MIN_VALUE || result > Integer.MAX_VALUE) { | ||
| throw new PrestoException(NUMERIC_VALUE_OUT_OF_RANGE, "Overflow multiplying double by interval year-month value: " + left + " * " + right); |
There was a problem hiding this comment.
Why are we comparing the result with int_max here? The return type is long
There was a problem hiding this comment.
As I understand, like IntegerType, the type IntervalYearMonthType in presto specifies long.class as its javaType, but the actual primitive value is int. So the functions here need to declare long as their parameters and return types, but the actual values should be within the range of integers. This PR handles integer overflow scenarios by directly throwing expected exceptions rather than returning unexpected values after overflow.
pramodsatya
force-pushed
the
intervalym_1
branch
from
b7d8d8a to
cf87eaf
Compare
7 participants
Description
Part of #24087. Duplicate of #24089, continuing it here since Tim is OOO and this change is needed for Velox PR: facebookincubator/velox#11612.
Motivation and Context
Fix incorrect data caused by overflow, and correct the type mapping in certain operators.
Impact
Where previously some expressions could have resulted in an overflow and incorrect results, now they will fail with a
NUMERIC_VALUE_OUT_OF_RANGEerror.Test Plan
Unit tests
Release Notes